-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compléter les historiques de Déclarations #1482
Conversation
) | ||
) | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai du mal à comprendre ce code. Je pense que c'est surtout car je n'ai pas les infos sur ica/vrs, population cible vs population... mais j'ai une question et une suggestion:
Question: est-ce qu'il existe une risque de MultipleObjectsReturned pour les gets ?
Suggestion:
cible_populations = Ica...Declaree.objects.filter(...)
populations = [Population.objects.get(...) for population in cible_populations]
declaration.populations.set(populations)
et pour les conditions aussi. Après, je sais pas si cible_populations
est le bon nom de variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ça a été factorisé ailleurs, l291
) | ||
) | ||
] | ||
) | ||
declaration.save() | ||
nb_created_declarations += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
je me demande si c'est mieux de mettre ça au dessus de ligne 278 si jamais il y a une erreur pas attendu avec les set
s ?
declaration.save() | ||
nb_created_declarations += 1 | ||
|
||
except IntegrityError: | ||
# cette Déclaration a déjà été créée |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pour confirmer : si la déclaration est déjà créée, on veut pas MAJ les populations ni les conditions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oui et non.
Pour le moment cet import est plutôt pensé comme un import One-Shot.
Et même si je vais l'effectuer 2 fois, je pensais d'abord supprimer toutes les déclarations historiques (car elles ne peuvent pour le moment pas avoir été modifiées) pour les réimporter entièrement.
Mais c'est vrai que je pourrais envisager de juste mettre à jour des champs ManyToMany
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je dirais de faire au plus simple s'il n'y a pas de changement possible
@@ -68,7 +68,7 @@ class Meta: | |||
|
|||
dcl_ident = factory.Sequence(lambda n: n + 1) | |||
cplalim = factory.SubFactory(ComplementAlimentaireFactory) | |||
tydcl_ident = factory.Faker("pyint", min_value=0, max_value=20) | |||
tydcl_ident = factory.Faker("pyint", min_value=0, max_value=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pourquoi le changement ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dans TeleIcare, ce champ ne peut avoir que 3 valeurs (0, 1, 2) car il n'y à que trois types (Art 15, Art 16, Simplifiée)
class Meta: | ||
managed = False | ||
db_table = "ica_population_cible_declaree" | ||
unique_together = (("vrsdecl_ident", "popcbl_ident"),) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Est-ce que c'est possible d'avoir plusieurs lignes avec le même vrsdecl_ident
alors ? Je demande car "primary_key=True implies null=False and unique=True." - https://docs.djangoproject.com/en/5.2/ref/models/fields/#primary-key
alors si il y a plusieurs qui sont les mêmes faut pas ajouter ligne 104 et 117 ?
Sinon, cette ligne unique_together
est superflu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La même remarque s'applique à IcaPopulationRisqueDeclaree
et IcaEffetDeclare
(donc ligne 130 aussi).
Si je comprends bien Téléicare calcule un ID unique en concaténant l'ID de la déclaration et celui de l'objet (population, effet, etc), mais nous on prend ces IDs dans des champs séparés ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bien vu. C'est bien juste le unique_together
qui est valide pour ces 3 tables. pas le primary_key=True.
Je modifie.
Comme ce sont des tables managed=True
, la définition du modèle selon Django n'est pas importante (et ne conditionne pas les contraintes de la table) c'est pour ça que ça n'avait pas fait d'erreur.
Mais c'est mieux que ça corresponde à la réalité.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix ici
# TODO: ces champs proviennent de tables pas encore importées | ||
# populations= | ||
# conditions_not_recommended= | ||
# other_conditions= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ces changements couvrent aussi other_conditions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pas encore, je le ferais dans une future PR
2c1cfca
to
b223c9c
Compare
Closes #1465
Cette PR permet d'importer certaines informations qui étaient manquantes dans les déclarations historiques, notamment :
Produit
Composition
TODO next :
Import de champs